fix: suppress osgetenvlibrary lint findings in pkg/cli and pkg/workflow#43967
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR suppresses osgetenvlibrary custom linter findings in non-main packages (pkg/cli, pkg/workflow) by adding targeted //nolint:osgetenvlibrary directives at environment-variable access call sites that are intended to run at entrypoint/CLI time.
Changes:
- Added call-site
//nolint:osgetenvlibrarysuppressions onos.Getenv/os.LookupEnvusage inpkg/cli/*andpkg/workflow/*. - Kept suppressions localized to the specific lines (no file-wide disables) to avoid masking unrelated findings.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/process_env_lookup.go | Suppresses the library-env lint for process env lookup helper (os.LookupEnv). |
| pkg/cli/update_check.go | Suppresses lint for MCP server detection env check. |
| pkg/cli/shell_completion.go | Suppresses lint for shell detection and Homebrew prefix env checks. |
| pkg/cli/secrets.go | Suppresses lint for checking secret presence via environment variables. |
| pkg/cli/secret_set_command.go | Suppresses lint for resolving secret values from an env var name. |
| pkg/cli/mcp_validation.go | Suppresses lint for validating/auto-resolving GitHub token env vars in MCP config validation. |
| pkg/cli/mcp_tools_privileged.go | Suppresses lint for reading GITHUB_REPOSITORY when constructing args. |
| pkg/cli/mcp_server_command.go | Suppresses lint for reading GITHUB_ACTOR for MCP server behavior. |
| pkg/cli/mcp_repository.go | Suppresses lint for preferring GITHUB_REPOSITORY when resolving repo context. |
| pkg/cli/logs_command.go | Suppresses lint for fast-path env repo comparison in logs flows. |
| pkg/cli/interactive.go | Suppresses lint for guarding interactive mode in test/CI via env vars. |
| pkg/cli/init.go | Suppresses lint for GHES detection via GITHUB_SERVER_URL / GH_HOST. |
| pkg/cli/import_url_fetcher.go | Suppresses lint for reading GH_HOST in import auth host selection. |
| pkg/cli/engine_secrets.go | Suppresses lint for engine secret discovery via env vars (including alternatives). |
| pkg/cli/compile_update_check.go | Suppresses lint for disabling compile update checks via env var. |
| pkg/cli/compile_orchestrator.go | Suppresses lint for GH host defaulting behavior gated by GH_HOST. |
| pkg/cli/codespace.go | Suppresses lint for Codespaces detection via env var. |
| pkg/cli/ci.go | Suppresses lint for CI detection via a set of env vars. |
| pkg/cli/add_wizard_command.go | Suppresses lint for CI gating of interactive add-wizard behavior. |
| pkg/cli/add_interactive_workflow.go | Suppresses lint for Codespaces gating of interactive run prompts. |
| pkg/cli/add_interactive_orchestrator.go | Suppresses lint for test/CI gating and GH host detection behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Low
| // Check for MCP_SERVER environment variable that could be set by the MCP server | ||
| return os.Getenv("GH_AW_MCP_SERVER") != "" | ||
| return os.Getenv("GH_AW_MCP_SERVER") != "" //nolint:osgetenvlibrary |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43967 does not have the 'implementation' label and has only 33 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Review: Lint suppression vs. the lookupEnv wrapper
No correctness or security issues. All suppressions are for legitimate entrypoint-time environment reads and follow the same //nolint:osgetenvlibrary pattern established in pkg/constants, pkg/logger, etc.
One non-blocking observation worth acting on:
pkg/cli has an injectable wrapper that covers most of these sites
pkg/cli/process_env_lookup.go already provides a lookupEnv(key string) string helper that routes through an overrideable processEnvLookup function (set to os.LookupEnv in production but swappable in tests). Two files in pkg/cli already use it (add_interactive_engine.go, outcomes_command.go).
Of the 32 nolint additions in pkg/cli, most could be replaced by calling lookupEnv(...) instead of os.Getenv(...) — no nolint needed, and tests gain env-override capability for free.
Suggested follow-up: replace os.Getenv → lookupEnv across pkg/cli and retire the nolint comments there. pkg/workflow and the other packages that already use suppression are fine as-is.
Overall the PR is mechanically correct and does not introduce regressions.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 51.4 AIC · ⌖ 5.78 AIC · ⊞ 4.8K
Comments that could not be inline-anchored
pkg/cli/engine_secrets.go:231
The pkg/cli package already has an injectable lookupEnv wrapper in process_env_lookup.go that routes through the overrideable processEnvLookup function — exactly the pattern intended to decouple this package from the process environment.
Using lookupEnv(req.Name) (and the equivalent at the other 31 os.Getenv call sites in pkg/cli) would fix the lint correctly and preserve test-time env injectability, rather than permanently suppressing the finding:
// Instead of:
envValu…
</details>
<details><summary>pkg/workflow/process_env_lookup.go:4</summary>
This wrapper suppresses the lint on the single `os.LookupEnv` call inside the package — but notice that `pkg/workflow/compilerenv/manager.go` lines 29 and 37 still pass `os.Getenv` as a function reference (not a call, so the analyzer does not flag them). `pkg/workflow/action_mode.go:73` similarly passes `os.Getenv` as a first-class function.
These are architecturally sound (they inject `os.Getenv` via the `EnvGetter` abstraction rather than calling it directly), so no suppression is needed the…
</details>There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — the approach is valid, but a few call sites can eliminate their suppressions entirely by using existing wrappers.
📋 Key Themes & Highlights
Key Themes
- Wrapper bypass:
IsRunningInCI()andisRunningInCodespace()already abstract the same env-var checks being suppressed inadd_interactive_orchestrator.go,add_wizard_command.go,interactive.go, andadd_interactive_workflow.go. Routing through those wrappers removes the suppressions and keeps detection logic in one place. - Bulk suppression approach: For the remaining 31 sites (secrets, host detection, MCP context, shell detection, update-check), the inline
//nolintapproach is the right pattern — it follows the established convention, is call-site-scoped (not file-wide), and is easy to audit. ✅ process_env_lookup.go: Correctly the designated abstraction point for env reads inpkg/workflow. ✅
Positive Highlights
- ✅ Call-site (not file-wide) suppressions — easy to audit and grep
- ✅ Consistent with the existing pattern in
pkg/constants,pkg/logger,pkg/envutil, etc. - ✅ No behavior changes — purely lint compliance
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 61.3 AIC · ⌖ 7.76 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| // Assert this function is not running in automated unit tests or CI | ||
| if os.Getenv("GO_TEST_MODE") == "true" || os.Getenv("CI") != "" { | ||
| if os.Getenv("GO_TEST_MODE") == "true" || os.Getenv("CI") != "" { //nolint:osgetenvlibrary |
There was a problem hiding this comment.
[/codebase-design] IsRunningInCI() already exists in ci.go — using it here removes two direct env reads and their suppressions.
💡 Suggested refactor
// Before
if os.Getenv("GO_TEST_MODE") == "true" || os.Getenv("CI") != "" { (nolint/redacted):osgetenvlibrary
// After — no suppression needed
if os.Getenv("GO_TEST_MODE") == "true" || IsRunningInCI() {This also means future CI vars added to IsRunningInCI() are automatically covered here.
@copilot please address this.
|
|
||
| // Assert this function is not running in automated unit tests | ||
| if os.Getenv("GO_TEST_MODE") == "true" || os.Getenv("CI") != "" { | ||
| if os.Getenv("GO_TEST_MODE") == "true" || os.Getenv("CI") != "" { //nolint:osgetenvlibrary |
There was a problem hiding this comment.
[/codebase-design] Same pattern as add_interactive_orchestrator.go — replace the raw os.Getenv("CI") check with the existing IsRunningInCI() wrapper to keep CI-detection logic in one place.
💡 Suggested refactor
// Before
if os.Getenv("GO_TEST_MODE") == "true" || os.Getenv("CI") != "" { (nolint/redacted):osgetenvlibrary
// After
if os.Getenv("GO_TEST_MODE") == "true" || IsRunningInCI() {Both files guard against the same conditions; centralising removes the duplication.
@copilot please address this.
| // add-wizard requires an interactive terminal | ||
| isTerminal := tty.IsStdoutTerminal() | ||
| isCIEnv := os.Getenv("CI") != "" | ||
| isCIEnv := os.Getenv("CI") != "" //nolint:osgetenvlibrary |
There was a problem hiding this comment.
[/codebase-design] IsRunningInCI() already encapsulates this check — use it to eliminate the suppression and avoid duplicating CI-detection logic.
💡 Suggested refactor
// Before
isCIEnv := os.Getenv("CI") != "" (nolint/redacted):osgetenvlibrary
// After
isCIEnv := IsRunningInCI()@copilot please address this.
|
|
||
| // In Codespaces, don't offer to trigger - provide link to Actions page instead | ||
| if os.Getenv("CODESPACES") == "true" { | ||
| if os.Getenv("CODESPACES") == "true" { //nolint:osgetenvlibrary |
There was a problem hiding this comment.
[/codebase-design] isRunningInCodespace() in codespace.go encapsulates this exact check — delegating to it would remove the suppression and centralise Codespace detection.
💡 Suggested refactor
// Before
if os.Getenv("CODESPACES") == "true" { (nolint/redacted):osgetenvlibrary
// After
if isRunningInCodespace() {The existing codespace.go already has the //nolint annotation; this avoids spreading the same check across multiple files.
@copilot please address this.
There was a problem hiding this comment.
❌ Wrong fix: 32 pkg/cli sites should use lookupEnv(), not //nolint:osgetenvlibrary
pkg/cli already has a mutex-protected lookupEnv() wrapper backed by an injectable processEnvLookup function variable in process_env_lookup.go. The osgetenvlibrary rule is correctly flagging real violations of that design. Suppressing them with //nolint silences the linter without fixing the underlying problem.
Blocking findings
1. 32 suppressions in pkg/cli defeat the testability seam (high)
Every suppressed os.Getenv call permanently bypasses processEnvLookup. Tests that override env behavior through that seam will silently miss all 32 sites. The fix is one line per site: replace os.Getenv(x) with lookupEnv(x) — no new abstraction needed.
2. Security-sensitive reads in engine_secrets.go bypass the wrapper (high)
The five os.Getenv calls reading secret names and token values are now hardwired to the real process environment. Tests asserting secret-resolution behavior must mutate the live process env, risking parallel-test interference and accidental token exposure in CI logs.
3. pkg/workflow/process_env_lookup.go suppression is correct — that is the designated wrapper file; the nolint there is appropriate.
Recommended action: Revert the 32 pkg/cli nolint annotations and replace each os.Getenv(x) call with lookupEnv(x). The single nolint on pkg/workflow/process_env_lookup.go can stay.
🔎 Code quality review by PR Code Quality Reviewer · 130.8 AIC · ⌖ 7.46 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/engine_secrets.go:234
All 5 secret env reads bypass the injectable lookupEnv() wrapper and cannot be overridden in tests.
<details>
<summary>💡 Suggested fix</summary>
pkg/cli already has process_env_lookup.go with a mutex-protected lookupEnv(key string) string wrapper and an injectable processEnvLookup function variable — designed exactly for this. Instead of suppressing the lint diagnostic, use the wrapper:
// Before (suppressed with nolint)
envValue := os.Getenv(req.Name) (nolint/redacted):…
</details>
<details><summary>pkg/workflow/process_env_lookup.go:341</summary>
**The nolint suppression on `pkg/workflow/process_env_lookup.go` is correct, but the 32 suppressions added to `pkg/cli` files are the wrong fix** — `pkg/cli` already has its own `lookupEnv()` wrapper with an injectable `processEnvLookup` variable.
<details>
<summary>💡 Why this matters</summary>
`pkg/cli/process_env_lookup.go` defines:
```go
var (
processEnvLookupMu sync.RWMutex
processEnvLookup envLookupFunc = os.LookupEnv
)
func lookupEnv(key string) string { ... }This inj…
pkg/cli/ci.go:21
IsRunningInCI() reads env vars directly and cannot be faked in tests without t.Setenv clobber — should use lookupEnv().
<details>
<summary>💡 Suggested fix</summary>
// Before
if os.Getenv(v) != "" { (nolint/redacted):osgetenvlibrary
// After
if lookupEnv(v) != "" {lookupEnv() is already present in pkg/cli/process_env_lookup.go and routes through the injectable processEnvLookup function — meaning tests can swap out env behavior without mutating the real process en…
|
🎉 This pull request is included in a new release. Release: |
35
osgetenvlibrarycustom lint findings flagged directos.Getenv/os.LookupEnvcalls in non-main library packages (pkg/cli,pkg/workflow). All reads are legitimate entrypoint-time environment checks (CI detection, shell detection, GitHub Actions context vars, host overrides, secret resolution).Changes
//nolint:osgetenvlibraryat each of the 35 call sites across 21 files, following the pattern already established inpkg/constants,pkg/logger,pkg/console,pkg/parser, andpkg/envutilAffected files:
pkg/workflow/process_env_lookup.go, and 20 files underpkg/cli/includingengine_secrets.go,shell_completion.go,mcp_validation.go,init.go, and others listed in the issue.